Confluence as source plugin #5404
Conversation
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
san81
requested review from
sb2k16,
chenqi0805,
engechas,
srikanthjg,
graytaylor0,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
|
|
||
| @Override | ||
| public void initCredentials() { | ||
| //do nothing for basic authentication |
There was a problem hiding this comment.
Should there be default implementation with empty function? That way you do not need it here.
There was a problem hiding this comment.
Agree. Implemented the change
| /** | ||
| * Initializes the credentials for the Jira instance. | ||
| */ | ||
| void initCredentials(); |
There was a problem hiding this comment.
Adding empty default implementation may be helpful here. Something to consider.
There was a problem hiding this comment.
Agree. Implemented the change
| */ | ||
| public class Constants { | ||
|
|
||
| public static final int RETRY_ATTEMPT = 6; |
There was a problem hiding this comment.
nit: should this be "MAX_RETRIES"?
|
|
||
| @Override | ||
| public String getPartitionKey() { | ||
| return space + "|" + contentType + "|" + UUID.randomUUID(); |
There was a problem hiding this comment.
Is this well known convention? If not, why "space"?
There was a problem hiding this comment.
"space" in the confluence terms, is a name used to indicate set of pages or blogspot. Like for example, all documentation related to product management goes into "Product Management Space"
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
| * Exception to indicate unauthorized access. | ||
| * It could either be caused by invalid credentials supplied by the user or failed renew the credentials. | ||
| */ | ||
| public final class UnAuthorizedException extends RuntimeException { |
| implementation 'com.fasterxml.jackson.core:jackson-databind' | ||
| implementation 'javax.inject:javax.inject:1' | ||
| implementation 'org.jsoup:jsoup:1.18.3' | ||
| implementation("org.springframework:spring-web:${libs.versions.spring.get()}") |
There was a problem hiding this comment.
We should just move spring-web into the libs file in the settings. Then we can clean these up.
| } | ||
| } | ||
|
|
||
| test { |
There was a problem hiding this comment.
You don't need this. It is inherited.
| testImplementation project(path: ':data-prepper-test-common') | ||
| } | ||
|
|
||
| test { |
There was a problem hiding this comment.
You don't need this. It is inherited.
| * Jira account url | ||
| */ | ||
| @JsonProperty("hosts") | ||
| protected List<String> hosts; |
There was a problem hiding this comment.
Why is this a list of hosts? It seems the code only supports one host and that would be simpler for users:
host: https://myaccount.atlassian.com
There was a problem hiding this comment.
It is a recommendation from Raj to support future expansion I guess.
| } | ||
|
|
||
|
|
||
| public static boolean validateConfig(ConfluenceSourceConfig config) { |
There was a problem hiding this comment.
This should be done in the configuration classes themselves.
| * The type Confluence configuration. | ||
| */ | ||
| @Slf4j | ||
| public class ConfluenceConfigHelper { |
There was a problem hiding this comment.
I think all of these things can be moved into the configuration classes themselves to have consistent access into them.
| int total; | ||
| int startAt = 0; | ||
| do { | ||
| ConfluenceSearchResults searchContentItems = confluenceRestClient.getAllContent(cql, startAt); |
There was a problem hiding this comment.
There may be some CQL injection issues. You will either need to do proper CQL escaping, or add validations to ensure that the values only have alphanumeric. I think you could write some odd queries by include ) in the queries.
There was a problem hiding this comment.
Individual filter items are validated either with a regex or checking against the expected enum values before the placed into the cql construction.
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
Description
Adding Confluence as a source based on the common SAAS Source-crawler framework. Turns out this PR is a little big in size because I introduced Atlassian common module to store the code related Jira and Confluence at one place. Some of the classes that actually reusable across other saas crawlers are moved to
source-crawlerThe code that is common between Jira and Confluence is the way how we authenticate to Atlassian cloud. In this PR, I didn't touch jira source code yet, not to further increase the PR size. I will give a follow up PR to make use ofatlassian-commonsin the jira source codeIssues Resolved
Introducing Confluence as a source
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.